Bugfix: Removed @RequirePost and changed ssh key migration behavior #498
Conversation
| @@ -1066,7 +1065,6 @@ public ListBoxModel doFillSshKeysCredentialsIdItems(@QueryParameter String sshKe | |||
| .includeCurrentValue(sshKeysCredentialsId); | |||
| } | |||
|
|
|||
| @RequirePOST | |||
There was a problem hiding this comment.
It looks like fine to remove this annotation here, the method is just returning whether the credential-id showed is an actual rsa private key. But better to confirm with some security person: @daniel-beck, @Wadeck, ...
There was a problem hiding this comment.
There's still a permission check.
The docs for this are at https://www.jenkins.io/doc/developer/security/form-validation/ -- please read and if anything specific is unclear, ask.
There was a problem hiding this comment.
getSshCredential retrieves credentials, but as the RequirePOST protects against CSRF, there is no issue. The method is already checking for ADMINISTER.
Side note, such methods should not have troubles to use POST for the check. You can change the
checkMethod="post"
MRamonLeon
requested review from
res0nance,
varyvol,
Wadeck,
daniel-beck and
MRamonLeon
|
@markyjackson-taulia Please read the comment above about the RequirePOST ;)
Be sure to test your code before commiting, there is no "manual test" performed everytime on PR and this could lead to broken release sometimes ;) |
… where they are necessary
|
Now I tested the migration manually after all changes maybe we should release that as a hotfix or something before more updates fail. |
|
@MRamonLeon @res0nance @daniel-beck Tests are passing and even if no test was created to verify the issue, @djesionek has tested it manually. Should we maybe release this and work in the test later? |
|
I think releasing as a bugfix release is reasonable. waiting to see if there are any other comments |
|
I think that if really we've had people manually verifying the fix, we may want to merge, then do a followup PR for an automated test. However, given the relative ease of use of |
|
I've been trying to create an automated test with @LocalData but I've not been able so far: the clouds are not loaded by Jenkins. But I've manually verified this is working properly: the credential is created and assigned to the existing cloud and the verification for the SSH key works now. |
|
See #500 for an automated test. |
I changed the behavior from my feature added in #445 as suggested by @daniel-beck by moving the migration code to EC2Cloud.readResolve(). I also tested again the migration from 1.51 to 1.52. I could confirm that it wasn't working before and that it works now for me with that change.
Not sure why the @RequirePost lead to issues as it was requested as an addition but maybe someone can clarify if there is a better way than just removing it for now.
Thanks to @varyvol for the bugnotice and @daniel-beck for the hint how to fix it quickly.